Initial version of crate #1
Conversation
| @@ -0,0 +1,24 @@ | |||
| /* This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
These build tests are currently incomplete. Missing tests include fields of types which don't implement the appropriate traits, fields of types which don't implement the appropriate traits but do have identically-named methods, and probably a battery of tests to validate that all of our happy-path cases compile properly (although this is kind of handled by the integration tests as well).
There was a problem hiding this comment.
I haven't fully reviewed the patch yet (I've reviewed most of the property processing and some of the code generation), but wanted to submit this to smooth out the work a bit (rather than landing one huge review weeks later).
Overall my main issue is that I find the code quite lacking in terms of comments. imo comments should not only be used to explain potentially tricky bits of the code, but also to provide a human-readable explanation of what a given portion does to guide someone through their first look reading the code, since parsing code requires much more effort than reading a comment that summarises what it does. This is especially true from an accessibility point of view: contributors using screen readers will have the code read one line at a time, and might not be able to grasp the context of a given line as easily as sighted folks; having an explanation of what the code does beforehand helps provide this context.
xml_struct_tests/ui/test_cases/type_properties/no_properties.rs
Outdated
Show resolved
Hide resolved
leftmostcat
force-pushed
the
initial-import
branch
from
b5dad65 to
435eecc
Compare
I'd love to add additional comments; the biggest difficulty here is, of course, that I wrote the code and know what it does. I'd greatly appreciate it if you could leave a quick note anywhere where you feel that understanding what's happening is non-trivial. |
There was a problem hiding this comment.
The crate looks good to me from a functional point of view (I have used it without any issue and haven't seen noticed significant functional issue during my review), so most of my comments relate to documentation and readability. I haven't gotten around to looking at the tests crate yet, I'll look at it in a future round.
| @@ -12,6 +12,7 @@ use quick_xml::{ | |||
|
|
|||
| use crate::{Error, XmlSerialize, XmlSerializeAttr}; | |||
|
|
|||
| /// Serializes a string as a text content node. | |||
There was a problem hiding this comment.
nitpick (non-blocking): I think it could be helpful to mention how it interacts with the Writer it borrows from the consumer, i.e what event(s) each impl send (or doesn't). Same for the otherimpl blocks.
There was a problem hiding this comment.
Raises an interesting question of where the line is between implementation details and visible side effects. Given how easy autogenerated Rust docs make it to access the source, I'm going to lean toward "describe in comments, let them check source if they really need details".
There was a problem hiding this comment.
where the line is between implementation details and visible side effects
fwiw I think that if the writer is given to us by the consumer, then it means the consumer might want to write more stuff into it, in which case what's already in there should be easy to infer from the documentation.
There was a problem hiding this comment.
I'm actually going to approve this at this stage, because a) its good work and b) i far prefer incremental development.
Please do consider the simplification and robustness points around errors though, for more information see: https://gist.github.com/quad/a8a7cc87d1401004c6a8973947f20365
It's my hope that the documentation in the crate is sufficient that most things here will be self-explanatory. Please do ask questions if it isn't, because then I can make my docs better.